Added parameter to sd.concatenate to control whether to merge coordinate systems with the same name#919
Conversation
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #919 +/- ##
==========================================
+ Coverage 92.19% 92.20% +0.01%
==========================================
Files 49 49
Lines 7550 7560 +10
==========================================
+ Hits 6961 6971 +10
Misses 589 589
🚀 New features to boost your workflow:
|
…uffix-to-coordinate-systems
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new merge_coordinate_systems_on_name flag to sd.concatenate, allowing users to control whether coordinate systems with the same name are merged or given distinct suffixes.
Key changes:
- Add
merge_coordinate_systems_on_nameparameter toconcatenateAPI and forward it to the renaming helper. - Update
_fix_ensure_unique_element_namesto propagate transformations and apply suffixes (or not) based on the new flag. - Add a parametrized test in
test_concatenate_merge_coordinate_systems_on_nameto verify both behaviors.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/core/test_concatenate.py | New test test_concatenate_merge_coordinate_systems_on_name checks merged vs. suffixed CS names. |
| src/spatialdata/_core/concatenate.py | - Added merge_coordinate_systems_on_name to signature and doc.\n- Enhanced _fix_ensure_unique_element_names to handle CS transformations. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new parameter, merge_coordinate_systems_on_name, to sd.concatenate in order to control whether coordinate system names are merged (i.e. kept unchanged) or suffixed with a key.
- Adds merge_coordinate_systems_on_name to both the public concatenate function and the internal _fix_ensure_unique_element_names helper.
- Updates the test suite to verify that coordinate system names are handled as expected based on the parameter value.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/core/test_concatenate.py | New test verifying the behavior of merge_coordinate_systems_on_name in concatenated outputs. |
| src/spatialdata/_core/concatenate.py | Updates to function signatures and implementation to support the merge_coordinate_systems_on_name flag. |
| modify_tables_inplace: bool, | ||
| merge_coordinate_systems_on_name: bool, | ||
| ) -> list[SpatialData]: | ||
| elements_by_sdata: list[dict[str, SpatialElement]] = [] |
src/spatialdata/_core/concatenate.py
Outdated
| # Handle coordinate systems and transformations | ||
| for element_name, element in elements.items(): | ||
| # Get the original element from the input sdata | ||
| original_name = element_name.replace(f"-{suffix}", "") |
There was a problem hiding this comment.
no need for this, we already know el when constructing the elements dict.
src/spatialdata/_core/concatenate.py
Outdated
| raise TypeError(f"Expected 'transformations' to be a dict, but got {type(transformations).__name__}.") | ||
|
|
||
| # Remove any existing transformations from the new element | ||
| remove_transformation(element, remove_all=True) |
There was a problem hiding this comment.
This code removes all the transformations, then checks (at every iteration) for the value of merge_coordinate_system_on_name, and when it is true it restores the original transformation.
This is convoluted and note needed, as one could check merge_coordinate_system_on_name before calling remove_transformation.
tests/core/test_concatenate.py
Outdated
| @@ -0,0 +1,36 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
All the tests for concatenate are in test_spatialdata_operations.py, I will move this code there.
|
@timtreis I gave comments on the code and committed the suggestions that I made during review. Please have a look at the changes and if you are happy with them we can merge. |
…uffix-to-coordinate-systems
…uffix-to-coordinate-systems
LucaMarconato
deleted the
bugfix/issue918-sdconcatenate-doesnt-apply-suffix-to-coordinate-systems
branch
2 participants
No description provided.